Build the shared Base UI control family - #86
Conversation
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head b31a9a8d93352569ba2a7f49ad2c584e16ce5af4 against base f3c9be6ce79decace5c384366710bd69d301ca2e.
Changes requested
P2: Route Textarea’s control props through Field.Control
src/shared/design-system/ui/Textarea.tsx:8-11
All caller props are placed on the rendered <textarea>, leaving Base UI unaware of its explicit id. For example, <Field label="Notes"><Textarea id="notes-control" /></Field> renders a label targeting Base UI’s generated control ID, while the actual textarea has id="notes-control". Clicking the label does not focus the field in either Chromium or WebKit. aria-labelledby still supplies an accessible name, so the existing role-query/typing test misses the broken native association.
Pass the relevant control props to BaseField.Control, leaving textarea-specific attributes on its rendered element as needed, and add coverage for an explicit ID with label-click focus. Preserve refs and controlled edits while doing so.
P2: Keep uncontrolled choice state consistent after form reset
src/shared/design-system/ui/Checkbox.tsx:17-23 and src/shared/design-system/ui/RadioGroup.tsx:12
The new wrappers expose defaultChecked / defaultValue and native form participation, but the installed Base UI 1.7.0 path leaves visible state and submitted values inconsistent after a native reset. Reproduction: start with a checked checkbox and radio value a, uncheck the checkbox and choose b, then activate a native <button type="reset">. In both Chromium and WebKit, the visible/ARIA state remains unchecked and b, while new FormData(form) contains the checked checkbox and a.
This is a dependency behavior exposed by the new shared controls, not a production feature regression at this layer. Resolve reset synchronization at the primitive boundary (including an upstream fix if appropriate) and add a regression that checks both visible checked state and FormData after reset. The current ordinary-change test does not exercise reset.
P2: Keep the new invalid field outline above 3:1
src/shared/design-system/styles/forms.css:42-44
The new invalid-state rule replaces the normal control outline with --border-danger. In light mode, both browser engines resolve this to rgb(235, 142, 144) against the field’s white --surface-panel, only 2.385:1. Since the field fill is also the panel fill, that border is the visible control boundary; the error text does not restore its visibility. This fails the documented 3:1 requirement in DESIGN.md:256-261.
The token defect is inherited from #85 and already reported there; #86 introduces its actual Input/Textarea consumption. Resolve the shared role in the foundation and bring that correction through the stack rather than adding a local palette override. Cover the rendered invalid boundary in both themes.
Validation and scope
- Focused browser probes at the reviewed head used the real shared components and both host/viewer CSS entry points in Chromium and WebKit. Loading blocked pointer, Enter/Space and implicit submission; re-enabling produced one action and one submission with unchanged button dimensions. Search clearing restored input focus. The explicit-ID textarea failure and invalid-border colors reproduced in both engines.
- The new form CSS is not imported by the host at #86, but these wrappers have no feature consumers in this snapshot and #88 explicitly adds the import when adopting them. This is a stack integration dependency, not an additional blocker here. The deliberate shared radius/size changes are not being treated as regressions merely because old assertions changed.
- Hosted JavaScript and both Chromium shards passed. WebKit shard 1/2 failed in
navigation-scroll-intent.spec.mjson a relay-stream access-control console error (fixture.mjs:1027); I have not established causation from this diff. CI remains red. Broad CI-equivalent suites and native visual testing were not rerun locally.
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head b91c8fbb6d5d1cc4fa9cf876ff3e79a02d26268a against stacked base 27dd5d0d590943abe748c5c25b9b423d33ed67a2 (#85, not main).
Changes requested
P2: Isolate the new control radius from unmigrated host controls
src/shared/design-system/styles/tokens.css:641 changes --radius-control from var(--corner-control) (12px at the default scale) to 0.5rem (8px). That is intentional for the new shared controls, but the unchanged host bridges still map both --radius-legacy-control and Tailwind’s --radius-xl to it (src/shared/styles/tokens.css:46, src/shared/styles/globals.css:56). Unmigrated native buttons and rounded-xl controls therefore also change from 12px to 8px.
This affects real legacy consumers, including the profile inputs at src/features/communities/ProfileFields.tsx:24,38 and plugin-folder choices at src/app/PluginImport.tsx:190. The unchanged compatibility contract explicitly preserves existing native-control recipes and separates the old control radius (docs/design-system.md:138-145). This is distinct from the intended geometry changes to shared Button/fields.
The existing reproduction is the browser case “compiled host preserves compatibility utility meanings”: its #old-primary native button uses rounded-xl. This PR changes that compatibility expectation from 12px to 8px at tests/browser/appearance.spec.mjs:276-279, accepting the leak rather than preserving the old surface.
Smallest fix: retain the shared 8px radius, point the host’s legacy radius at the retained --corner-control value, route its legacy rounded-xl mapping through that legacy radius, and restore the #old-primary expectation to 12px. No downstream migration or broader radius redesign is needed.
Prior findings and validation limits
- The explicit-ID Textarea correction is present, with label-focus/ref/controlled-edit coverage. Invalid Input/Textarea boundaries now consume the corrected foundation danger role. The reset patch and regression cases are present; independent review found no additional demonstrated blocking reset defect, with ESM/CJS additions and patch-lock references consistent.
- Source-only review on the pinned Blox object store, including wrappers, style entrypoints, compatibility consumers, changed tests and the dependency patch. Existing exact-head CI snapshot: 12/12 checks successful. I did not execute repository code/tests or reproduce browser/desktop behavior. Installed upstream Base UI implementation details outside the patch context remain a validation limit; this is not a blanket runtime certification of reset semantics.
- Choice-invalid border styling is not an additional blocker: no governing requirement establishes a mandatory red outline, and Field provides error text. Downstream host form adoption remains outside stack 2/4.
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
…tings Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: the prior P2 compatibility defect remains
Reviewed afc4de9a603178442aa4f16d0e884043586cfe43 against exact stacked base a8deea467f5d6c5700d61f7aa76fdce61814301a (PR #85), not main. This refresh preserves the previously reviewed PR-specific files; it does not fix the finding from review 5272664839.
P2: Isolate the new control radius from unmigrated host controls
src/shared/design-system/styles/tokens.css:641 still changes --radius-control from var(--corner-control) (12px at the default root size) to 0.5rem (8px). The unchanged host aliases still point to that shared value:
src/shared/styles/tokens.css:46:--radius-legacy-control: var(--radius-control).src/shared/styles/globals.css:56:--radius-xl: var(--radius-control).
Consequently, the new shared-control geometry also reaches the native-control recipes and legacy rounded-xl utilities. Real consumers include the profile inputs at src/features/communities/ProfileFields.tsx:24,38 and plugin-folder choices at src/app/PluginImport.tsx:190. This contradicts the retained compatibility contract at docs/design-system.md:138–145, rather than merely reflecting the intended 8px shared-control design.
The existing browser case “compiled host preserves compatibility utility meanings” contains the reproduction: #old-primary uses rounded-xl, but this PR changes its expected radius from 12px to 8px (tests/browser/appearance.spec.mjs:276–279). That accepts the compatibility change instead of guarding against it.
Same smallest fix and exit criteria: preserve the shared 8px radius; point --radius-legacy-control at the retained --corner-control, route the legacy --radius-xl through that legacy radius, and restore the #old-primary expectation to 12px. No downstream migration or radius redesign is required.
Re-review scope and validation
Two complementary source-only lanes completed on the pinned Blox object store. The PR-specific changed files match the previous reviewed head; the Textarea identity correction, corrected invalid-field role, and reset patch/regressions remain unchanged. No additional blocking reset or field-identity defect was established, and those accepted paths were not broadly re-reviewed.
Existing hosted CI is 12/12 successful at this exact head. No repository code was checked out, installed, built, tested, imported or executed during this review. The CSS chain and changed assertion above are source evidence, not a new browser measurement or desktop acceptance. No approval, dismissal or merge was performed.
Resolve foundation, mention, registry and browser conflicts without changing shared-control behavior. Preserve pinned typography provenance and main's local-agent shapes. Keep 8px shared corners separate from 12px host compatibility aliases. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Pushed Shared corners remain 8px; legacy host controls/ Validation: 2,003 unit tests in the normal pre-push hook, 59 design tests, 38 viewer + 34 host-browser checks across Chromium/WebKit, app/viewer production builds, types/design guards and frozen install passed. Checked tree: |
| diff --git a/checkbox/root/CheckboxRoot.js b/checkbox/root/CheckboxRoot.js | ||
| index 7000305591bd64e9f26ed573e12ee41a873cafe4..52cfbef7a269d4abcdfe8311b42269750ee15529 100644 | ||
| --- a/checkbox/root/CheckboxRoot.js | ||
| +++ b/checkbox/root/CheckboxRoot.js | ||
| @@ -7,6 +7,7 @@ Object.defineProperty(exports, "__esModule", { | ||
| }); | ||
| exports.PARENT_CHECKBOX = exports.CheckboxRoot = void 0; | ||
| var React = _interopRequireWildcard(require("react")); | ||
| +var _useStableCallback = require("@base-ui/utils/useStableCallback"); | ||
| var _empty = require("@base-ui/utils/empty"); | ||
| var _useControlled = require("@base-ui/utils/useControlled"); | ||
| var _useIsoLayoutEffect = require("@base-ui/utils/useIsoLayoutEffect"); | ||
| @@ -139,6 +140,37 @@ const CheckboxRoot = exports.CheckboxRoot = /*#__PURE__*/React.forwardRef(functi |
There was a problem hiding this comment.
I'm surprised we have to patch React here. Do we really need this?
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Shared buttons, fields, text inputs, textareas, checkboxes and radio groups use the semantic colors introduced in #85. Base UI owns interaction and form behavior; Buzz owns the common visual treatment. The design viewer shows the supported states and variants.
CI assertion repair:
5c92029Commit
5c920292220921d99eb5af11ee3347ebef4a4a0d, tree8f5a3ac1f6343d94a56951513a5e8473408b42e7, repairs the two inherited browser assertions exposed by the failed integrated run. Each failed in both Chromium and WebKit; the completed run contained no other failing browser test. TheCI requiredfailure aggregates those four journey shards.plugin-import.spec.mjsandagent-control.spec.mjsin both engines: 4 failures/32 passes at cleana6a5969(55.3s), then 36/36 passes on this commit's exact tree (37.6s). Command:bin/pnpm test:browser tests/browser/plugin-import.spec.mjs tests/browser/agent-control.spec.mjs --project chromium --project webkit --no-deps. Local Apple Silicon macOS, two workers, zero retries. Browser cases added/removed: 0/0. All wrapping, containment, square-icon, draft, operation, fallback and error assertions remain. Independent bounded source/diff review is clear; it is not PR approval.New hosted CI is pending; this is not merge readiness. GitHub currently reports APPROVED from the existing review on
a6a5969; the repair author did not submit an approval or dismiss any review. The explicit decision about maintaining the Base UI patch remains unresolved. No approval, review dismissal, merge, or #87–88 changes. Non-blocking follow-up: the viewer's IconButton prose incomponentSpecimens.tsxstill describes old hit-area dimensions; it was not folded into this CI assertion repair. No new native GUI/package acceptance is claimed.Earlier main integration:
a6a5969Targets main at
4ac257e1b2b2b9ca68f413eccc5baf6b3804060f, after #85 merged. Merge commita6a5969cc8761edc84db941f78e6e17a4644eac5resolves the ten conflicts without rewriting history. The checked tree isa492180350896e668f0c9cee4df190071ac43f62.rounded-xlretain 12px. The existing compiled-host assertion is restored, not relaxed. Chromium and WebKit both failed with received 8px before the fix, then passed with 12px afterward.This integration adds/removes zero browser cases; it restores an existing compiled-CSS regression. The five prior viewer scenarios below remain unchanged. Broad hosted CI was in progress at that delivery and subsequently failed the two inherited assertions repaired above; existing requested-change reviews still require re-review. This is not approval, merge authorization, native GUI/release acceptance, or a claim that all CI passed. #87 and #88 were not changed.
Earlier validation history
Originally stack 2/4; #85 is now merged and this PR targets main. It uses the current shared Phosphor icon gateway.
Review fixes:
Further review caught reset cases involving initially disabled or late-mounted radios and controlled owners whose values do not change during reset. The patch now binds to each real input, reads the current owner values, and synchronizes after the browser's native reset action. Canceled resets and unmounted controls do not apply queued work. No wrapper state, remount, or synthetic change event is introduced.
The refreshed button label also exposed wrapping in the existing compound agent-activity row. A small feature-layout adjustment keeps its avatar, name, status and indicator together. Browser expectations now account for the shared button's 1px border and semantic dark hover fill; tolerances and interaction checks remain intact.
Validation at
b91c8fb:Five viewer scenarios are added by the review fixes (ten engine cases): actual textarea label focus, native reset/submitted values, computed invalid-boundary contrast, controlled reset without an owner change, and radios enabled after mount. These prove native focus/form integration and compiled CSS that jsdom cannot. Larger state matrices remain in mounted tests; no browser cases or assertions were removed. The new reset regressions and existing narrow-layout checks failed before the fixes. All hosted checks passed at
b91c8fb: JavaScript, Rust/tool integration, Windows notifications, browser measurements, all Chromium/WebKit journey shards, security, DCO and CI required. Desktop GUI acceptance remains separate; existing requested-change reviews need reviewer re-review.Latest base refresh at
afc4de9merges #85a8deea4, including current main through0b73a45. The merge was clean and preserves the inherited presence, native Dock badge, and local-development notification mute behavior. No #86-specific source changes were needed. Normal commit/push hooks pass: TypeScript, 1,879 unit tests across 181 files (18.49 seconds), viewer typechecking, and all design guards. The remote head matches this checked commit; every commit against the PR base has a sign-off, and hosted DCO passes. All 12 hosted checks now pass at this exact head, including every Chromium/WebKit shard, native/tool checks, measurements, security and DCO. The browser/build evidence above belongs to its stated earlier snapshot; it was not rerun locally for this clean parent merge.